Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update OpenTelemetry Tracing version status #13808

Closed
wants to merge 7 commits into from
Closed

Update OpenTelemetry Tracing version status #13808

wants to merge 7 commits into from

Conversation

hanyuancheung
Copy link

@hanyuancheung hanyuancheung commented Mar 16, 2022 •
edited
Loading

Follow: #12929
Fixs: #12460
Noticed that OpenTelemetry has released some stable versions as well as new features. Now update its version, and remove the experimental status in tracing config.

@ahrtr
Copy link
Member

ahrtr commented Mar 16, 2022

Overall Looks good to me, please add an entry into 3.6 CHANGELOG.

We also need an agreement as discussed in issues/13775.

serathius
serathius requested changes Mar 16, 2022
View reviewed changes
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before marking distributed tracing as a stable feature we need to agree on expectations of how stable tracing for etcd should look like. Does it work? Do we have enough tests? Is tracking used withing Etcd codebase?

Would be good to discuss it with the K8s tracing owner @dashpole

@dashpole
Copy link

An integration test similar to kubernetes/kubernetes#103234 would be useful. @hanyuancheung have you gotten any user feedback from trying to use the feature, or know of successful use anywhere?

@houseme
Copy link

houseme commented Mar 17, 2022

Follow: #12929 Fixs: #12460 Noticed that OpenTelemetry has released some stable versions as well as new features. Now update its version, and remove the experimental status in tracing config.

The latest release is 1.5.0.
https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.5.0

@ahrtr
Copy link
Member

ahrtr commented Mar 17, 2022

An integration test similar to kubernetes/kubernetes#103234 would be useful. @hanyuancheung have you gotten any user feedback from trying to use the feature, or know of successful use anywhere?

Thanks for the info. etcd needs to add similar integration test. Just raised an issue to track this.

@hanyuancheung
Copy link
Author

An integration test similar to kubernetes/kubernetes#103234 would be useful. @hanyuancheung have you gotten any user feedback from trying to use the feature, or know of successful use anywhere?

Thanks for the info. etcd needs to add similar integration test. Just raised an issue to track this.

I agree. This PR should focus on updating otel tracing version to the newest one, and I've noticed @ahrtr has opened the issue to track the addition integration test for etcd tracing.

cc @serathius @dashpole @houseme @ahrtr

@hanyuancheung
Copy link
Author

hanyuancheung commented Mar 19, 2022 •
edited
Loading

I'll also work on #13814 after this one finished, to make sure the feature is useful with the integration test 😄 Thx for ur suggestion!

@hanyuancheung hanyuancheung requested a review from serathius March 19, 2022 10:42
serathius
serathius reviewed Mar 19, 2022
View reviewed changes
server/embed/config.go Outdated
// Can only be used if ExperimentalEnableDistributedTracing is true.
ExperimentalDistributedTracingServiceName string `json:"experimental-distributed-tracing-service-name"`
// ExperimentalDistributedTracingServiceInstanceID is the ID key of the service.
// EnableDistributedTracing indicates if tracing using OpenTelemetry is enabled.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a separate PR to stabilize distributing tracing feature. We still have some work before stabilization.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get it. So this PR should just work on updating the version of OpenTelemetry right?
If so, I'll cancel the changes of all stabilization work ~

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can update OpenTelemetry now and decide separably to stabilize the tracing.

@hanyuancheung hanyuancheung requested a review from serathius March 21, 2022 01:51
@hanyuancheung @serathius
Update CHANGELOG/CHANGELOG-3.6.md
88e08d0 
Co-authored-by: Marek Siarkowicz <[email protected]>
serathius
serathius approved these changes Mar 21, 2022
View reviewed changes
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ptabor @spzala

@codecov-commenter
Copy link

Codecov Report

Merging #13808 (88e08d0) into main (1b208aa) will decrease coverage by 0.64%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #13808      +/-   ##
==========================================
- Coverage   72.72%   72.07%   -0.65%     
==========================================
  Files         467      467              
  Lines       38280    38280              
==========================================
- Hits        27839    27591     -248     
- Misses       8662     8883     +221     
- Partials     1779     1806      +27
Flag Coverage Δ
all 72.07% <0.00%> (-0.65%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
server/embed/config_tracing.go 18.33% <0.00%> (ø)
client/v3/snapshot/v3_snapshot.go 0.00% <0.00%> (-52.09%) ⬇️
etcdctl/ctlv3/command/compaction_command.go 30.43% <0.00%> (-43.48%) ⬇️
etcdctl/ctlv3/command/alarm_command.go 51.35% <0.00%> (-27.03%) ⬇️
client/v3/txn.go 73.33% <0.00%> (-26.67%) ⬇️
server/etcdserver/cindex/cindex.go 82.35% <0.00%> (-11.77%) ⬇️
server/storage/quota.go 57.89% <0.00%> (-9.22%) ⬇️
server/auth/simple_token.go 80.00% <0.00%> (-8.47%) ⬇️
client/pkg/v3/tlsutil/tlsutil.go 83.33% <0.00%> (-8.34%) ⬇️
server/storage/mvcc/watchable_store.go 84.42% <0.00%> (-8.34%) ⬇️
... and 35 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b208aa...88e08d0. Read the comment docs.

@serathius serathius mentioned this pull request Apr 5, 2022
Merged
@ahrtr
Copy link
Member

ahrtr commented Jun 23, 2022

The google.golang.org/grpc was already bumped to 1.47 in 14125, but the go.opentelemetry.io/otel wasn't bumped in that PR.

Could you rebase this PR and bump the go.opentelemetry.io/otel?

@houseme
Copy link

houseme commented Jun 25, 2022

The google.golang.org/grpc was already bumped to 1.47 in 14125, but the go.opentelemetry.io/otel wasn't bumped in that PR.

Could you rebase this PR and bump the go.opentelemetry.io/otel?

88cd0fe
In this commit 88cd0fe, otel is already the latest version

@ahrtr
Copy link
Member

ahrtr commented Jun 25, 2022

Thanks @houseme for the confirmation.

@ahrtr ahrtr closed this Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serathius serathius serathius approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hanyuancheung @ahrtr @dashpole @houseme @codecov-commenter @serathius